-
Notifications
You must be signed in to change notification settings - Fork 562
DB Query Engine #2265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DB Query Engine #2265
Conversation
Discovered a flaw in sqlstore's handling of wildcard perms during testing, adjusted Adjustd the s3 store to enable testing with this definition instead of separate mock Added tests for different aspects of the query engine, especially rbac and unit management.
taco/cmd/statesman/main.go
Outdated
| analytics.InitGlobalWithSystemID("production", store) | ||
|
|
||
| // 3. Create the base OrchestratingStore | ||
| orchestratingStore := storage.NewOrchestratingStore(blobStore, queryStore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this orchestrating store is grouping blobStore and queryStore which are both unrelated it seems, just use UnitStore and QueryStore independently ?
taco/internal/domain/interfaces.go
Outdated
| // ============================================ | ||
|
|
||
| // TFEOperations defines what TFE handler needs. | ||
| // TFE needs to auto-create workspaces but not list/delete/version operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this true that we "auto-create" I thought that we dont auto-create but rather we require them to be created before any operation on them would succeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah thats a good catch i'll confirm the behaviour now and adjust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do prevent auto create in most but there was one where not it turns out (createstateversiondirect), I'll adjust both that method and the comment
| @@ -0,0 +1,601 @@ | |||
| # Test Suite Guide - Quick Reference | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this doc adding any value? maybe an explanation on running the tests but what's summarising the number of tests will immediately go out of sync with the code. I would remove all but example commands on how to run the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah thats a good point, sort of made it for myself initially but through that lens it should probably be minimal i agree
| @@ -0,0 +1,67 @@ | |||
| package observability | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite understand this and the other sync methods in query/internal/sql_store.go
motatoes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need a workflow to run the test suite in here on every pr open
Adds modular DB backing, allow bucketstore abstraction.
Note: a large amount of the LOC change is the test suite and data.